-
Notifications
You must be signed in to change notification settings - Fork 9
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor module #49
Open
ksiamro-netz
wants to merge
16
commits into
Reviewscouk:master
Choose a base branch
from
ksiamro-netz:refactorModule
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Refactor module #49
+565
−367
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…_snippet_element.phtml
bartoszkubicki
added a commit
to WeareJH/reviews-co-uk-magento2-module
that referenced
this pull request
Jul 15, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Hey, I received request to check the module code and provide some info how to write it better. I also refactored the code a bit. Especially the Controller Classes and one of the Observer.
Basically, code standards for Magento2 can be found here:
https://devdocs.magento.com/guides/v2.3/coding-standards/bk-coding-standards.html
Which is bare minimum imo.
In addition:
https://developer.adobe.com/commerce/php/best-practices/extensions/
For better understanding Magento and rules:
https://developer.adobe.com/commerce/php/development/
https://developer.adobe.com/commerce/frontend-core/guide/
In addition, some Libs can be used to check code due to the standards. For example
https://github.com/magento/magento-coding-standard
https://github.com/PHP-CS-Fixer/PHP-CS-Fixer
https://github.com/squizlabs/PHP_CodeSniffer
Those libs will help with keep the code more structured and more clean. Obviously, the libs should be called manually to check code.
Of course, would be great to have also Unit and Integration tests.
Please read my comments, usually mentioned as
#
.Please add PhpDocs everywhere and in general comply with PSRs.
Important note: Please move the Observer for
sales_order_shipment_save_after
event, to Cronjob.Currently, every product in order may be loaded twice, which may greatly increase the process time. Using Cron, we would move the responsibility to another,
external
process, that will be ran in the background. To mark order, that were processed, You may add a new column tosales_oprder
table.Big help would be to use PHPStorm with installed plugins like PhpStan, Plugin for Magento, or even PHP Inspection.
PHPStorm can display deprecated method and may show badly written code. To fully use the functionality, try to work with the module on an already running shop instance locally, with access to the Magento Core Code.
Moreover, consider using Interfaces for Services or Models. Especially for the API Model, which connects to Review service.
In PHP Files, do not use
use
for catalogs. Define whole path to required Class. It will add some additional lines inuse
section, but it is safer and better to work with.Consider moving repeatable code to another class like Model or so.
Adapt code to use PHP in version 8.1 at least. Lower PHP versions are not supported anymore, therefore there is no sense to support it.
Do not load products one by one if not required. Every Product load will require connection to DB. With for few products is no issue, but makes a lot of trouble with many products. Product Collection can be used to load multiple products faster. For loading one entity use Repositories, no Models.
In Feed Controller, there may be problem with timeouts anyway. Especially for shops with thousands or a hundred thousand products.
I think about 2 solutions now:
Do not use ob_start and set_time_limit. If You need
set_time_limit(0)
, it means, the code is wrong written and optimized. In the worst case, not only the Timeout may be thrown, but also it may create some issues with the shop, if the code creates high loads and chains of loads and so on.Additionally, when You create code, keep in mind scalability -> there are places where, behavior will be different per shop. For example, collecting prices or availability of product, or even, when the Order is Closed / Delivered.
Create events or functions to use plugins for managing some behavior if necessary.
Otherwise, it may be necessary for developers to override the whole classes with whole code, to just change one line to change the small behavior and adapt it to shop requirements (which basically is not best approach).
On top of that, please keep module.xml file updated as well as
require
in compose.json.Also, its okay to move some code to another private functions. Using proper naming and PhpDocs it may make the code more clear. One big methods that do everything usually are not good approach. Using more methods also helps with working with the code for another developer and may help with using plugins or so.
Do not use ObjectManager to create objects. I can not image situation, when You really need to use it. Comply with injecting required classes in __construct(), even if will be used just once.